Refactor file path resolution for entitlements #127040
Conversation
jdconrad
added
>refactoring
v9.1.0
:Core/Infra/Entitlements
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -203,8 +200,8 @@ private static PolicyManager createPolicyManager() { | |||
| FileData.ofPath(Path.of("/proc/self/mountinfo"), READ).withPlatform(LINUX), | |||
| FileData.ofPath(Path.of("/proc/diskstats"), READ).withPlatform(LINUX) | |||
| ); | |||
| if (bootstrapArgs.pidFile() != null) { | |||
| serverModuleFileDatas.add(FileData.ofPath(bootstrapArgs.pidFile(), READ_WRITE)); | |||
| if (pathLookup.resolveRelativePaths(PID, Path.of("")) != null) { | |||
There was a problem hiding this comment.
This is an interesting case. We want to know if a PID file was specified. Could we do this within resolution in the pathlookup? In this case, if there is no pid file, there should be no path resolved (though a file data would exist for it).
There was a problem hiding this comment.
I agree. Special cased this to have a getter method.
| @@ -166,9 +168,9 @@ private FileAccessTree( | |||
| } | |||
|
|
|||
| // everything has access to the temp dir, config dir, to their own dir (their own jar files) and the jdk | |||
| addPathAndMaybeLink.accept(pathLookup.tempDir(), READ_WRITE); | |||
| addPathAndMaybeLink.accept(pathLookup.getBaseDirPaths(TEMP).findFirst().get(), READ_WRITE); | |||
There was a problem hiding this comment.
Don't we need to add all of the possible temp dirs? Eg for an ESIntegTestCase, each virtual node could have its own temp dir. Same for config below.
| Path tempDir, | ||
| Function<String, Stream<String>> settingResolver | ||
| ) {} | ||
| // TODO: (jack) new a need interface, but can still be a record |
There was a problem hiding this comment.
The todo seems like it can be removed, but a simple javadoc would be good
|
|
||
| @Override | ||
| public Stream<Path> getBaseDirPaths(BaseDir baseDir) { | ||
| if (baseDir == BaseDir.HOME) { |
There was a problem hiding this comment.
Could this be a switch on baseDir? Then there is no need for the else case because we can ensure all values of the enum are covered.
|
|
||
| @Override | ||
| public Stream<Path> resolveRelativePaths(BaseDir baseDir, Path relativePath) { | ||
| if (baseDir == BaseDir.HOME) { |
There was a problem hiding this comment.
Same here, a switch would make this more readable
| } else if (baseDir == BaseDir.SHARED_REPO) { | ||
| return Arrays.stream(sharedRepoDirs).map(sharedRepoDir -> sharedRepoDir.resolve(relativePath)); | ||
| } else if (baseDir == BaseDir.CONFIG) { | ||
| return Stream.of(configDir.resolve(relativePath)); |
There was a problem hiding this comment.
These all seem to basically map to what is returned from getBaseDirPaths. Can we delegate to that and then map the return to resolve with the relative path?
| .filter(s -> s.toLowerCase(Locale.ROOT).startsWith("https://") == false) | ||
| .distinct() | ||
| .map(Path::of); | ||
| if (baseDir == BaseDir.HOME) { |
There was a problem hiding this comment.
Switch would be nicer here. It's unfortunate the combination is inverted. While we could reuse getBaseDirPaths, we lose the knowledge there is only a single base dir, and so would have to do the paths combination as with data dirs...
There was a problem hiding this comment.
Tried to clean this up a bit.
| return relativePaths.map(logsDir::resolve); | ||
| } else if (baseDir == BaseDir.TEMP) { | ||
| return relativePaths.map(tempDir::resolve); | ||
| } else if (baseDir == BaseDir.PID) { |
There was a problem hiding this comment.
PID seems like a special case. It's not a dir, so maybe it doesn't belong in the enum. Rather, it could be a special method just for pid on PathLookup, or have a separate enum for special files? And in that case we don't want any resolution, it's more just lookup up the (potentially many? in the test case) file paths for that constant.
| @@ -437,7 +438,7 @@ public void checkFileWrite(Class<?> callerClass, Path path) { | |||
| } | |||
|
|
|||
| public void checkCreateTempFile(Class<?> callerClass) { | |||
| checkFileWrite(callerClass, pathLookup.tempDir()); | |||
| checkFileWrite(callerClass, pathLookup.getBaseDirPaths(TEMP).findFirst().get()); | |||
There was a problem hiding this comment.
Could we have a comment here explaining why this is "ok"? in production we only ever have 1 temp dir, and this is really a production check on entitlements.
|
@rjernst Thank you for the feedback. I tried to clean up the code as much as possible based on your comments. |
| var writeAccessForbidden = pathSet(configDir); | ||
| static void validateFilesEntitlements(Map<String, Policy> pluginPolicies, PathLookup pathLookup) { | ||
| var readAccessForbidden = pathSet( | ||
| pathLookup.getBaseDirPaths(PLUGINS).findFirst().get(), |
There was a problem hiding this comment.
nit: maybe have a helper method to get a dir we expect, so that there can be a clear assertion (there should be exactly one of these in the stream, never 0 or 2+).
There was a problem hiding this comment.
I made this handle the streams properly instead of assuming there's only 1.
| */ | ||
| public interface PathLookup { | ||
| enum BaseDir { | ||
| HOME, |
There was a problem hiding this comment.
nit: I think this could be confused with Elasticsearch home directory, can we call this USER_HOME?
| TEMP | ||
| } | ||
|
|
||
| Path getPIDFile(); |
There was a problem hiding this comment.
trivially, if this is just pidFile then the prod impl will implicitly implement it, no need for an override.
This change refactors the known directory resolution such as modules, plugins, lib, etc. into a PathLookup. This is one of the steps towards allowing unit tests to provide their own PathLookup for resolution so we can enable entitlements there. ES-11584
This change refactors the known directory resolution such as modules, plugins, lib, etc. into a
PathLookup. This is one of the steps towards allowing unit tests to provide their ownPathLookupfor resolution so we can enable entitlements there.ES-11584